Add serial test execution support#125
Conversation
Allow designating specific tests to run sequentially instead of in parallel, useful for memory-hungry tests that cannot safely overlap. New `serial` and `serial_position` keyword arguments on `runtests` control which tests run one-at-a-time and whether the serial phase runs before (default) or after the parallel batch. A new exported `partition_tests` helper splits the ordered test list into serial and parallel groups. Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Claude <noreply@anthropic.com>
|
I like this interface. The only reason I could see for a more complicated interface is for designating testsets to be run on the same worker (mapreduce and mapreduce_large as a concrete example) to save on compilation but I don't think that's really worth the complexity. |
`integration/c` should not be run.
Co-authored-by: Claude <noreply@anthropic.com>
I pushed a change to always run the serial tests on a single worker because that was something I had in mind anyway: it's a bit silly and wasteful to spawn one worker per test, and when there are multiple serial tests the other workers remain sitting lazy after they're started. I also added a few more tests, to try and cover all corner cases I could think of (and I'm sure there are more!) |
|
I pushed d69b22d to make serial tests run at the end actually run sequentially with a single worker alive. Honestly I haven't looked at the source code very carefully (that was developed by Claude), but the tests are mine and those should be good and meaningful. I'll review the change when I have some more time (looks a bit overcomplicated for my taste), I'm a bit swamped at the moment, but I wanted to save the change plus relevant tests for the time being. |
|
FWIW this works for my desired use-cases. That is, tests marked as serial missing from test set (I don't run the big tests on CI by default), and they still get run serially when explicitly run using |
|
Thanks for testing! I just want to review the source code changes in d69b22d (tests were mine), I didn't have the time to look at that yet, that's why I put this on hold. |
|
I mentioned it because I wasn't sure if this was intentional behaviour since |
So, the error is from Claude, and I wasn't sure as well precisely for the case of filtering tests from the command line. I think that's a case of excessively defensive code (for the standalone function |
Ok, I finally found the time to look at this. That's more complicated than what I was hoping for, but the code does what it was intended to do (it's stopping the extra parallel workers before the serial part starts, so that only one worker remains alive, without clogging memory needlessly) and I don't have smart ideas to simplify this, so I think this is ready for review |
2 participants
This is my (first?) attempt to fix #77 (CC @christiangnrd).
I'm not married to this implementation, I was originally thinking of changing the values in the
testsuitedictionary to be a struct wrappingExpr, and with a field to designate a serial test, but then I had a friendly chat with Claude and it suggested as a less breaking alternative adding a keyword argument toruntests. I let Claude write the first implementation, I didn't like it and I changed it. I'm happy to consider other options.See section about serial execution in the docs preview.
PR best reviewed by ignoring whitespace changes.